Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@morganbr
Copy link

Adds serialization support to BigInteger and Complex, which were inadvertently dropped in the previous cleanup. Also renames the private fields of Complex to match NetFX so that it will be serialization compatible. Testing will be covered in an upcoming change by @ViktorHofer.

This is progress on #19119

morganbr added 2 commits May 23, 2017 22:20
…vetently dropped in the previous cleanup. Also renames the private fields of Complex to match NetFX so that it will be serialization compatible.
@morganbr morganbr added this to the 2.1.0 milestone May 24, 2017
@morganbr morganbr self-assigned this May 24, 2017
@morganbr
Copy link
Author

CC @danmosemsft @dotnet/corert-contrib

_real = real;
_imaginary = imaginary;
m_real = real;
m_imaginary = imaginary;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to commentthat field names must not change, ornot

Copy link

@sharwell sharwell May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Assuming the upcoming test covers it, I wouldn't think the comment would be necessary. Seems like adding the comments is prone to a situation where one or more comments is omitted, and a reader incorrectly infers that the lack of a comment means the name can change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer we add comments. I understand @sharwell's point, but at the same time, seeing a comment and avoiding a change entirely is a lot quicker than having to figure out why a test is failing, can be spotted in code reviews, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something inline like

            m_real = real; // Do not rename (binary serialization)
            m_imaginary = imaginary; // Do not rename (binary serialization)

We already do stuff like this in types that are hard bound to the runtime eg on string and array, although I guess there will be many more like this.
Your call @morganbr

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go ahead with those comments in this change, but leave the overall process to @ViktorHofer and @krwq when they handle all of the other types.

@morganbr morganbr merged commit 9966578 into dotnet:master May 24, 2017
@morganbr morganbr deleted the NumericsSerialization branch May 24, 2017 21:59
krwq pushed a commit to krwq/corefx that referenced this pull request May 31, 2017
Adds serialization support to BigInteger and Complex, which were inadvetently dropped in the previous cleanup. Also renames the private fields of Complex to match NetFX so that it will be serialization compatible.
krwq pushed a commit to krwq/corefx that referenced this pull request Jun 1, 2017
Adds serialization support to BigInteger and Complex, which were inadvetently dropped in the previous cleanup. Also renames the private fields of Complex to match NetFX so that it will be serialization compatible.
aignas added a commit to aignas/mathnet-numerics that referenced this pull request Aug 21, 2017
It seems that serialization support for `Complex` in
`System.Runtime.Numerics` got cleaned up a while ago.  It is
scheduled for reintroduction in `netstandard2.1`.

See the following PR for more info:
dotnet/corefx#20222
aignas added a commit to aignas/mathnet-numerics that referenced this pull request Nov 2, 2017
It seems that serialization support for `Complex` in
`System.Runtime.Numerics` got cleaned up a while ago.  It is
scheduled for reintroduction in `netstandard2.1`.

See the following PR for more info:
dotnet/corefx#20222
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants